Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[React18] Migrate test suites to account for testing library upgrades security-detection-engine #201149

Merged

Conversation

eokoneyo
Copy link
Contributor

@eokoneyo eokoneyo commented Nov 21, 2024

This PR migrates test suites that use renderHook from the library @testing-library/react-hooks to adopt the equivalent and replacement of renderHook from the export that is now available from
@testing-library/react. This work is required for the planned migration to react18.

Context

In this PR, usages of waitForNextUpdate that previously could have been destructured from renderHook are now been replaced with waitFor exported from @testing-library/react, furthermore waitFor
that would also have been destructured from the same renderHook result is now been replaced with waitFor from the export of @testing-library/react.

Why is waitFor a sufficient enough replacement for waitForNextUpdate, and better for testing values subject to async computations?

WaitFor will retry the provided callback if an error is returned, till the configured timeout elapses. By default the retry interval is 50ms with a timeout value of 1000ms that
effectively translates to at least 20 retries for assertions placed within waitFor. See https://testing-library.com/docs/dom-testing-library/api-async/#waitfor for more information.
This however means that for person's writing tests, said person has to be explicit about expectations that describe the internal state of the hook being tested.
This implies checking for instance when a react query hook is being rendered, there's an assertion that said hook isn't loading anymore.

In this PR you'd notice that this pattern has been adopted, with most existing assertions following an invocation of waitForNextUpdate being placed within a waitFor
invocation. In some cases the replacement is simply a waitFor(() => new Promise((resolve) => resolve(null))) (many thanks to @kapral18, for point out exactly why this works),
where this suffices the assertions that follow aren't placed within a waitFor so this PR doesn't get larger than it needs to be.

It's also worth pointing out this PR might also contain changes to test and application code to improve said existing test.

What to do next?

  1. Review the changes in this PR.
  2. If you think the changes are correct, approve the PR.

Any questions?

If you have any questions or need help with this PR, please leave comments in this PR.

@eokoneyo eokoneyo added release_note:skip Skip the PR/issue when compiling release notes backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) Team:Detection Engine Security Solution Detection Engine Area React@18 labels Nov 21, 2024
@eokoneyo eokoneyo self-assigned this Nov 21, 2024
@eokoneyo
Copy link
Contributor Author

/ci

@eokoneyo eokoneyo force-pushed the chore/testing-library-migration-for-security-detection-engine branch from 9b0f81d to 861764d Compare November 22, 2024 12:06
@eokoneyo
Copy link
Contributor Author

/ci

@eokoneyo eokoneyo marked this pull request as ready for review November 22, 2024 14:15
@eokoneyo eokoneyo requested review from a team as code owners November 22, 2024 14:15
@eokoneyo eokoneyo requested a review from vitaliidm November 22, 2024 14:15
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-detection-engine (Team:Detection Engine)

@eokoneyo
Copy link
Contributor Author

@elasticmachine merge upstream

@botelastic botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Nov 25, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

}

try {
abortCtrl = new AbortController();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

won't every new call of saveExceptionItem create a new instance of AbortController?
That would lead to replacing value in abortCtrl and older requests might not be cancelled when component gets unmounted.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to test this, but turned out this hook is not used anymore in code.

@eokoneyo, would you mind to delete it and associated test? Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

won't every new call of saveExceptionItem create a new instance of AbortController? That would lead to replacing value in abortCtrl and older requests might not be cancelled when component gets unmounted.

Yes every request has it's own AbortController especially that if the component gets unmounted we call abort on the signal passed, in fact this approach is quite similar to the previous implementation with the only difference being that the controller is only initialised when we know that an actual request will be made.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to test this, but turned out this hook is not used anymore in code.

@eokoneyo, would you mind to delete it and associated test? Thanks

I think this is outside the scope of this PR, I've however created an issue for this here #201917

@eokoneyo
Copy link
Contributor Author

@elasticmachine merge upstream

2 similar comments
@eokoneyo
Copy link
Contributor Author

@elasticmachine merge upstream

@eokoneyo
Copy link
Contributor Author

eokoneyo commented Dec 2, 2024

@elasticmachine merge upstream

@eokoneyo eokoneyo enabled auto-merge (squash) December 2, 2024 10:26
@eokoneyo
Copy link
Contributor Author

eokoneyo commented Dec 4, 2024

@elasticmachine merge upstream

fireEvent.change(renderResult.getByDisplayValue('/var/log/nginx/access.log*'), {
target: { value: '' },
});
await waitFor(() => Promise.resolve(null));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you share the explanation on why waitFor(() => Promise.resolve(null)) this is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @juliaElastic The waitFor(() => Promise.resolve(null)) has been added to account for instances where an async operation is performed so we can resolve whatever action might have been queued before proceeding the next part of the test. I've noticed that in this particular test suite, the test can also pass without including this, there's other parts of this test that absolutely require this, that been said it's really your call since you have more knowledge about this part of the code if we should keep it or not.

Copy link
Contributor

@juliaElastic juliaElastic Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use this anywhere else in Fleet tests, so I don't think it's needed. Could you try removing from this file and see if the test passes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @juliaElastic I removed the usages in this file, however there were some places that still required it.

@eokoneyo eokoneyo force-pushed the chore/testing-library-migration-for-security-detection-engine branch from bd8274a to 4b4c982 Compare December 10, 2024 15:43
@eokoneyo eokoneyo force-pushed the chore/testing-library-migration-for-security-detection-engine branch from 4b4c982 to fe1bfe5 Compare December 11, 2024 11:38
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

cc @eokoneyo

@eokoneyo eokoneyo merged commit 3683cc2 into main Dec 11, 2024
8 checks passed
@eokoneyo eokoneyo deleted the chore/testing-library-migration-for-security-detection-engine branch December 11, 2024 13:46
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/12277683108

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Dec 11, 2024
… security-detection-engine (elastic#201149)

This PR migrates test suites that use `renderHook` from the library
`@testing-library/react-hooks` to adopt the equivalent and replacement
of `renderHook` from the export that is now available from
`@testing-library/react`. This work is required for the planned
migration to react18.

##  Context

In this PR, usages of `waitForNextUpdate` that previously could have
been destructured from `renderHook` are now been replaced with `waitFor`
exported from `@testing-library/react`, furthermore `waitFor`
that would also have been destructured from the same renderHook result
is now been replaced with `waitFor` from the export of
`@testing-library/react`.

***Why is `waitFor` a sufficient enough replacement for
`waitForNextUpdate`, and better for testing values subject to async
computations?***

WaitFor will retry the provided callback if an error is returned, till
the configured timeout elapses. By default the retry interval is `50ms`
with a timeout value of `1000ms` that
effectively translates to at least 20 retries for assertions placed
within waitFor. See
https://testing-library.com/docs/dom-testing-library/api-async/#waitfor
for more information.
This however means that for person's writing tests, said person has to
be explicit about expectations that describe the internal state of the
hook being tested.
This implies checking for instance when a react query hook is being
rendered, there's an assertion that said hook isn't loading anymore.

In this PR you'd notice that this pattern has been adopted, with most
existing assertions following an invocation of `waitForNextUpdate` being
placed within a `waitFor`
invocation. In some cases the replacement is simply a `waitFor(() => new
Promise((resolve) => resolve(null)))` (many thanks to @kapral18, for
point out exactly why this works),
where this suffices the assertions that follow aren't placed within a
waitFor so this PR doesn't get larger than it needs to be.

It's also worth pointing out this PR might also contain changes to test
and application code to improve said existing test.

### What to do next?
1. Review the changes in this PR.
2. If you think the changes are correct, approve the PR.

## Any questions?
If you have any questions or need help with this PR, please leave
comments in this PR.

Co-authored-by: Elastic Machine <[email protected]>
(cherry picked from commit 3683cc2)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Dec 11, 2024
…grades security-detection-engine (#201149) (#203802)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[React18] Migrate test suites to account for testing library upgrades
security-detection-engine
(#201149)](#201149)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Eyo O.
Eyo","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-12-11T13:46:09Z","message":"[React18]
Migrate test suites to account for testing library upgrades
security-detection-engine (#201149)\n\nThis PR migrates test suites that
use `renderHook` from the library\r\n`@testing-library/react-hooks` to
adopt the equivalent and replacement\r\nof `renderHook` from the export
that is now available from\r\n`@testing-library/react`. This work is
required for the planned\r\nmigration to react18.\r\n\r\n##
Context\r\n\r\nIn this PR, usages of `waitForNextUpdate` that previously
could have\r\nbeen destructured from `renderHook` are now been replaced
with `waitFor`\r\nexported from `@testing-library/react`, furthermore
`waitFor`\r\nthat would also have been destructured from the same
renderHook result\r\nis now been replaced with `waitFor` from the export
of\r\n`@testing-library/react`.\r\n\r\n***Why is `waitFor` a sufficient
enough replacement for\r\n`waitForNextUpdate`, and better for testing
values subject to async\r\ncomputations?***\r\n\r\nWaitFor will retry
the provided callback if an error is returned, till\r\nthe configured
timeout elapses. By default the retry interval is `50ms`\r\nwith a
timeout value of `1000ms` that\r\neffectively translates to at least 20
retries for assertions placed\r\nwithin waitFor.
See\r\nhttps://testing-library.com/docs/dom-testing-library/api-async/#waitfor\r\nfor
more information.\r\nThis however means that for person's writing tests,
said person has to\r\nbe explicit about expectations that describe the
internal state of the\r\nhook being tested.\r\nThis implies checking for
instance when a react query hook is being\r\nrendered, there's an
assertion that said hook isn't loading anymore.\r\n\r\nIn this PR you'd
notice that this pattern has been adopted, with most\r\nexisting
assertions following an invocation of `waitForNextUpdate`
being\r\nplaced within a `waitFor`\r\ninvocation. In some cases the
replacement is simply a `waitFor(() => new\r\nPromise((resolve) =>
resolve(null)))` (many thanks to @kapral18, for\r\npoint out exactly why
this works),\r\nwhere this suffices the assertions that follow aren't
placed within a\r\nwaitFor so this PR doesn't get larger than it needs
to be.\r\n\r\nIt's also worth pointing out this PR might also contain
changes to test\r\nand application code to improve said existing
test.\r\n\r\n### What to do next?\r\n1. Review the changes in this
PR.\r\n2. If you think the changes are correct, approve the
PR.\r\n\r\n## Any questions?\r\nIf you have any questions or need help
with this PR, please leave\r\ncomments in this
PR.\r\n\r\nCo-authored-by: Elastic Machine
<[email protected]>","sha":"3683cc28469a0acf4beb05898060db34db3f2f90","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team:Fleet","v9.0.0","backport:prev-minor","Team:Detection
Engine","React@18"],"title":"[React18] Migrate test suites to account
for testing library upgrades
security-detection-engine","number":201149,"url":"https://github.com/elastic/kibana/pull/201149","mergeCommit":{"message":"[React18]
Migrate test suites to account for testing library upgrades
security-detection-engine (#201149)\n\nThis PR migrates test suites that
use `renderHook` from the library\r\n`@testing-library/react-hooks` to
adopt the equivalent and replacement\r\nof `renderHook` from the export
that is now available from\r\n`@testing-library/react`. This work is
required for the planned\r\nmigration to react18.\r\n\r\n##
Context\r\n\r\nIn this PR, usages of `waitForNextUpdate` that previously
could have\r\nbeen destructured from `renderHook` are now been replaced
with `waitFor`\r\nexported from `@testing-library/react`, furthermore
`waitFor`\r\nthat would also have been destructured from the same
renderHook result\r\nis now been replaced with `waitFor` from the export
of\r\n`@testing-library/react`.\r\n\r\n***Why is `waitFor` a sufficient
enough replacement for\r\n`waitForNextUpdate`, and better for testing
values subject to async\r\ncomputations?***\r\n\r\nWaitFor will retry
the provided callback if an error is returned, till\r\nthe configured
timeout elapses. By default the retry interval is `50ms`\r\nwith a
timeout value of `1000ms` that\r\neffectively translates to at least 20
retries for assertions placed\r\nwithin waitFor.
See\r\nhttps://testing-library.com/docs/dom-testing-library/api-async/#waitfor\r\nfor
more information.\r\nThis however means that for person's writing tests,
said person has to\r\nbe explicit about expectations that describe the
internal state of the\r\nhook being tested.\r\nThis implies checking for
instance when a react query hook is being\r\nrendered, there's an
assertion that said hook isn't loading anymore.\r\n\r\nIn this PR you'd
notice that this pattern has been adopted, with most\r\nexisting
assertions following an invocation of `waitForNextUpdate`
being\r\nplaced within a `waitFor`\r\ninvocation. In some cases the
replacement is simply a `waitFor(() => new\r\nPromise((resolve) =>
resolve(null)))` (many thanks to @kapral18, for\r\npoint out exactly why
this works),\r\nwhere this suffices the assertions that follow aren't
placed within a\r\nwaitFor so this PR doesn't get larger than it needs
to be.\r\n\r\nIt's also worth pointing out this PR might also contain
changes to test\r\nand application code to improve said existing
test.\r\n\r\n### What to do next?\r\n1. Review the changes in this
PR.\r\n2. If you think the changes are correct, approve the
PR.\r\n\r\n## Any questions?\r\nIf you have any questions or need help
with this PR, please leave\r\ncomments in this
PR.\r\n\r\nCo-authored-by: Elastic Machine
<[email protected]>","sha":"3683cc28469a0acf4beb05898060db34db3f2f90"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/201149","number":201149,"mergeCommit":{"message":"[React18]
Migrate test suites to account for testing library upgrades
security-detection-engine (#201149)\n\nThis PR migrates test suites that
use `renderHook` from the library\r\n`@testing-library/react-hooks` to
adopt the equivalent and replacement\r\nof `renderHook` from the export
that is now available from\r\n`@testing-library/react`. This work is
required for the planned\r\nmigration to react18.\r\n\r\n##
Context\r\n\r\nIn this PR, usages of `waitForNextUpdate` that previously
could have\r\nbeen destructured from `renderHook` are now been replaced
with `waitFor`\r\nexported from `@testing-library/react`, furthermore
`waitFor`\r\nthat would also have been destructured from the same
renderHook result\r\nis now been replaced with `waitFor` from the export
of\r\n`@testing-library/react`.\r\n\r\n***Why is `waitFor` a sufficient
enough replacement for\r\n`waitForNextUpdate`, and better for testing
values subject to async\r\ncomputations?***\r\n\r\nWaitFor will retry
the provided callback if an error is returned, till\r\nthe configured
timeout elapses. By default the retry interval is `50ms`\r\nwith a
timeout value of `1000ms` that\r\neffectively translates to at least 20
retries for assertions placed\r\nwithin waitFor.
See\r\nhttps://testing-library.com/docs/dom-testing-library/api-async/#waitfor\r\nfor
more information.\r\nThis however means that for person's writing tests,
said person has to\r\nbe explicit about expectations that describe the
internal state of the\r\nhook being tested.\r\nThis implies checking for
instance when a react query hook is being\r\nrendered, there's an
assertion that said hook isn't loading anymore.\r\n\r\nIn this PR you'd
notice that this pattern has been adopted, with most\r\nexisting
assertions following an invocation of `waitForNextUpdate`
being\r\nplaced within a `waitFor`\r\ninvocation. In some cases the
replacement is simply a `waitFor(() => new\r\nPromise((resolve) =>
resolve(null)))` (many thanks to @kapral18, for\r\npoint out exactly why
this works),\r\nwhere this suffices the assertions that follow aren't
placed within a\r\nwaitFor so this PR doesn't get larger than it needs
to be.\r\n\r\nIt's also worth pointing out this PR might also contain
changes to test\r\nand application code to improve said existing
test.\r\n\r\n### What to do next?\r\n1. Review the changes in this
PR.\r\n2. If you think the changes are correct, approve the
PR.\r\n\r\n## Any questions?\r\nIf you have any questions or need help
with this PR, please leave\r\ncomments in this
PR.\r\n\r\nCo-authored-by: Elastic Machine
<[email protected]>","sha":"3683cc28469a0acf4beb05898060db34db3f2f90"}}]}]
BACKPORT-->

Co-authored-by: Eyo O. Eyo <[email protected]>
CAWilson94 pushed a commit to CAWilson94/kibana that referenced this pull request Dec 12, 2024
… security-detection-engine (elastic#201149)

This PR migrates test suites that use `renderHook` from the library
`@testing-library/react-hooks` to adopt the equivalent and replacement
of `renderHook` from the export that is now available from
`@testing-library/react`. This work is required for the planned
migration to react18.

##  Context

In this PR, usages of `waitForNextUpdate` that previously could have
been destructured from `renderHook` are now been replaced with `waitFor`
exported from `@testing-library/react`, furthermore `waitFor`
that would also have been destructured from the same renderHook result
is now been replaced with `waitFor` from the export of
`@testing-library/react`.

***Why is `waitFor` a sufficient enough replacement for
`waitForNextUpdate`, and better for testing values subject to async
computations?***

WaitFor will retry the provided callback if an error is returned, till
the configured timeout elapses. By default the retry interval is `50ms`
with a timeout value of `1000ms` that
effectively translates to at least 20 retries for assertions placed
within waitFor. See
https://testing-library.com/docs/dom-testing-library/api-async/#waitfor
for more information.
This however means that for person's writing tests, said person has to
be explicit about expectations that describe the internal state of the
hook being tested.
This implies checking for instance when a react query hook is being
rendered, there's an assertion that said hook isn't loading anymore.

In this PR you'd notice that this pattern has been adopted, with most
existing assertions following an invocation of `waitForNextUpdate` being
placed within a `waitFor`
invocation. In some cases the replacement is simply a `waitFor(() => new
Promise((resolve) => resolve(null)))` (many thanks to @kapral18, for
point out exactly why this works),
where this suffices the assertions that follow aren't placed within a
waitFor so this PR doesn't get larger than it needs to be.

It's also worth pointing out this PR might also contain changes to test
and application code to improve said existing test.

### What to do next?
1. Review the changes in this PR.
2. If you think the changes are correct, approve the PR.

## Any questions?
If you have any questions or need help with this PR, please leave
comments in this PR.

Co-authored-by: Elastic Machine <[email protected]>
CAWilson94 pushed a commit to CAWilson94/kibana that referenced this pull request Jan 13, 2025
… security-detection-engine (elastic#201149)

This PR migrates test suites that use `renderHook` from the library
`@testing-library/react-hooks` to adopt the equivalent and replacement
of `renderHook` from the export that is now available from
`@testing-library/react`. This work is required for the planned
migration to react18.

##  Context

In this PR, usages of `waitForNextUpdate` that previously could have
been destructured from `renderHook` are now been replaced with `waitFor`
exported from `@testing-library/react`, furthermore `waitFor`
that would also have been destructured from the same renderHook result
is now been replaced with `waitFor` from the export of
`@testing-library/react`.

***Why is `waitFor` a sufficient enough replacement for
`waitForNextUpdate`, and better for testing values subject to async
computations?***

WaitFor will retry the provided callback if an error is returned, till
the configured timeout elapses. By default the retry interval is `50ms`
with a timeout value of `1000ms` that
effectively translates to at least 20 retries for assertions placed
within waitFor. See
https://testing-library.com/docs/dom-testing-library/api-async/#waitfor
for more information.
This however means that for person's writing tests, said person has to
be explicit about expectations that describe the internal state of the
hook being tested.
This implies checking for instance when a react query hook is being
rendered, there's an assertion that said hook isn't loading anymore.

In this PR you'd notice that this pattern has been adopted, with most
existing assertions following an invocation of `waitForNextUpdate` being
placed within a `waitFor`
invocation. In some cases the replacement is simply a `waitFor(() => new
Promise((resolve) => resolve(null)))` (many thanks to @kapral18, for
point out exactly why this works),
where this suffices the assertions that follow aren't placed within a
waitFor so this PR doesn't get larger than it needs to be.

It's also worth pointing out this PR might also contain changes to test
and application code to improve said existing test.

### What to do next?
1. Review the changes in this PR.
2. If you think the changes are correct, approve the PR.

## Any questions?
If you have any questions or need help with this PR, please leave
comments in this PR.

Co-authored-by: Elastic Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) React@18 release_note:skip Skip the PR/issue when compiling release notes Team:Detection Engine Security Solution Detection Engine Area Team:Fleet Team label for Observability Data Collection Fleet team v8.18.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants